Skip to content

Optimize rpow(x,n,base)#78

Open
k06a wants to merge 2 commits intosky-ecosystem:masterfrom
k06a:optimize/rpow
Open

Optimize rpow(x,n,base)#78
k06a wants to merge 2 commits intosky-ecosystem:masterfrom
k06a:optimize/rpow

Conversation

@k06a
Copy link
Contributor

@k06a k06a commented Oct 19, 2019 •
edited
Loading

  1. Optimize rpow(x,n,base) for the case x != 0 && n == 0 (291 gas vs 383 gas)

    switch x case 0 {switch n case 0 {z := b} default {z := 0}}
default {
   ...
}

    =>

    switch n case 0 { z := b }
default {
    switch x case 0 { z := 0 }
    default {
        ...
    }
}

  2. Simplify overflow check for squaring in rpow(x,n,base) (2355 gas vs 2388 gas for x = 2, n = 255)

    let xx := mul(x, x)
if iszero(eq(div(xx, x), x)) { revert(0,0) }

    =>

    let xx := mul(x, x)
if shr(128, x) { revert(0,0) }

@k06a k06a changed the title Optimize rpow(x,n,base) for the case x != 0 && n == 0 (291 gas vs 383 gas) Optimize rpow(x,n,base) Oct 19, 2019
@k06a k06a force-pushed the optimize/rpow branch 3 times, most recently from 8724783 to 7061ea9 Compare October 19, 2019 21:36
@livnev
Copy link
Contributor

livnev commented Oct 20, 2019

@kmbarry1 can you get these optimisations to pass the spec? 😛

@kmbarry1
Copy link
Contributor

kmbarry1 commented Oct 21, 2019

I will give it a try today!

@kmbarry1 kmbarry1 requested review from a team and kmbarry1 October 21, 2019 16:06
@kmbarry1
Copy link
Contributor

kmbarry1 commented Oct 21, 2019

Also, just to give "official" comment on this: looks good, but there are two things I want to check prior to merging:

  1. gas for x != 0 && n != 0 is not worsened (I don't think it is, looks like it should be at least the same, maybe better, but want to check explicitly)
  2. must successfully formally verify this

@gbalabasquer
Copy link
Contributor

gbalabasquer commented Oct 29, 2019

@kmbarry1 do we have any update on this regarding FV?

@kmbarry1
Copy link
Contributor

kmbarry1 commented Oct 29, 2019

Am getting through the backlog of changes that are definitely going in before analyzing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kmbarry1 kmbarry1 Awaiting requested review from kmbarry1

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@k06a @livnev @kmbarry1 @gbalabasquer